Skip to content
This repository has been archived by the owner on Oct 13, 2023. It is now read-only.

Implement forceSelection attribute #240

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from

Conversation

gRoussac
Copy link

@gRoussac gRoussac commented Jun 7, 2017

Hi,

Thanks for your nice work on this component.

As user story, we would like to prevent the closing of the dropdown menu on hitting "Enter" or "Tab" when nothing was selected in the dropdown. We could not find an option corresponding to that case or maybe we missed it in the doc, if so please any advise appreciated.

The pull request may not be perfect but you get the point I hope.

Best regards,

Greg

@oferh oferh changed the base branch from master to dev June 8, 2017 11:23
@oferh
Copy link
Owner

oferh commented Jun 9, 2017

Hi @gRoussac,

Thanks for the PR, it's a nice addition.
Will try to check it soon and if there are no issues will merge it.

@gRoussac
Copy link
Author

gRoussac commented Jun 9, 2017

Yes please check if blur event is also behaving accordingly (not going through handleSelection if there was no selection) Thanks ! success !

Copy link
Owner

@oferh oferh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long time it took me to review, it's a good idea but it requires some more work before it's completed.

@@ -99,7 +100,9 @@ export class CtrInput {
if (this.completer.hasHighlighted()) {
event.preventDefault();
}
this.handleSelection();
if (!this.forceSelection || this.completer._hasHighlighted) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completer._hasHighlighted is a private member please use completer.hasHighlighted()

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the check can be done inside handleSelection which will prevent the code duplication here and in KEY_TAB

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not remember precisely what I was trying to do with the TAB, I guess maybe leaving the menu opened on TAB but this would prevent from switching to a next input in the form, which is pretty much what should happen. Otherwise you need to tqke your mouse to leave the field, and would be some kind of autofocus on TAB.

So I guess only ENTER key should be concerned.

README.md Outdated
@@ -112,6 +112,7 @@ Add the following to `System.js` map configuration:
|overrideSuggested|If true will override suggested and set the model with the value in the input field.|boolean|No|false|
|openOnFocus|If true will open the dropdown and perform search when the input gets the focus.|boolean|No|false|
|fillHighlighted|If true will set the model with the value in the input field when item is highlighted.|boolean|No|true|
|forceSelection|If true will prevent from closing dropdown on Enter and Tab keys down if no selection was made.|boolean|No|true|
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default should be false

@@ -1,6 +1,6 @@
<div class="completer-holder" ctrCompleter>
<md-input-container class="completer-input">
<input mdInput ctrInput="clearSelected=clearSelected; overrideSuggested=overrideSuggested; fillHighlighted=fillHighlighted" [(ngModel)]="searchStr"
<input mdInput ctrInput="clearSelected=clearSelected; overrideSuggested=overrideSuggested; fillHighlighted=fillHighlighted; forceSelection=forceSelection" [(ngModel)]="searchStr"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'll be better to change one of the nativeCmp demos to show how it's working

@@ -135,6 +135,7 @@ export class CompleterCmp implements OnInit, ControlValueAccessor, AfterViewChec
@Input() public openOnFocus = false;
@Input() public initialValue: any;
@Input() public autoHighlight = false;
@Input() public forceSelection = false;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forceSelection should also be used in the component html template [forceSelection]="forceSelection"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a confusion thinking completer-cmp-md.html was the template. I was rushing at that time ×)

@oferh
Copy link
Owner

oferh commented Jun 24, 2017

@gRoussac when blur is triggered for other reasons, like mouse press outside the component, forceSelection is not checked

@gRoussac
Copy link
Author

gRoussac commented Sep 16, 2017

On blur forceSelection is not checked and should not be. You are just leaving the input, I do not really get was I was trying to say/secure with this blur, seems to work correctly with forceSelection enabled ( ie: close the menu, no point in preventing going out of the component here)

(clearUnselected option is I guess was I was missing for the blur part, which enables to force a selection in items or nothing; i.e having forceSelection to true and clearUnselected to false might not be very consistent in ux)

(Btw clearUnselected option seems to bug with ESC key fyi)

@gRoussac
Copy link
Author

gRoussac commented Sep 16, 2017

Not sure I pulled the right branch. This PR is update to master branch though
but was made against the dev one :trollface:.

@landworth
Copy link

Is it possible for someone to merge this PR with the master branch, to fix the annoying Enter issue! Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants